Skip to content

Conversation

michalChrobot
Copy link
Collaborator

@michalChrobot michalChrobot commented Sep 17, 2025

Purpose of this PR

This PR aims to address 3 problems that we currently have

  1. Quite often if no checks are required it's easy to undertest a PR which leads to simple whitespace errors and such. Because of that I created pr_minimal_required_checks which consists of PVP checks and standard check which I will require on all PRs as minimum
  2. Emma noticed that PRs are allowed to be merged when jobs are running but none yet failed. This is an issue created by ci: Setting up conditional CI runs depending on PR comment trigger #3580 and the fact that we want to run CI conditionally (for example we don't want to run it when we only have DOCS changes) so we had to remove PR trigger as required job in branch protection rules. This creates a problem that without required check PR is allowed to be merged until a test fails
  3. Another problem that I spotted is that Documentation~ folder is actually located inside package folder so this check implemented there won't work correctly either way

The solution that this PR proposes is implementation of 2 independent checks (triggered by expression trigger) where

  1. pr_minimal_required_checks will trigger on all PRs targetting develop or release branches. It consist of simplest PVP and standards checks
  2. pr_code_changes_checks will trigger on PRs that are modifying files under relevant package or project related paths (see expression trigger)

Those jobs WON'T be required by the branch protection rules but I introduced new GitHub action of Yamato PR Supervisor which WILL be required and how it works is that it monitors checks running on PRs and will fail if any of them fails or pass if all checks are green. In this way we can have a workflow that both runs only necessary tests but also gates PRs from merging too early.

Note that you can still use /ci ngo comment trigger to run the check on draft branch or branches not targeting develop nor release branches

After this is approved and merged I will add this check as required on PRs in branch protection rules

Jira ticket

N/A

Documentation

Comments were added to the jobs (soon I also plan to create doc describing our CI practices/learnings)

Testing & QA (How your changes can be verified during release Playtest)

I tested scenarios with code changes and without code changes and the workflow works as expected

Backports

Backported to develop in #3691

@michalChrobot michalChrobot self-assigned this Sep 17, 2025
@michalChrobot michalChrobot marked this pull request as ready for review September 17, 2025 12:27
@michalChrobot michalChrobot marked this pull request as draft September 17, 2025 12:40
@michalChrobot michalChrobot marked this pull request as ready for review September 19, 2025 10:44
@michalChrobot michalChrobot requested a review from a team as a code owner September 19, 2025 10:44
@NoelStephensUnity NoelStephensUnity enabled auto-merge (squash) September 23, 2025 14:35
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@NoelStephensUnity NoelStephensUnity merged commit c1725c2 into develop-2.0.0 Sep 23, 2025
10 checks passed
@NoelStephensUnity NoelStephensUnity deleted the test-for-all-checks-pass-before-merging branch September 23, 2025 14:36
michalChrobot added a commit that referenced this pull request Sep 23, 2025
…m merging when tests are running (#3682)

This PR aims to address 3 problems that we currently have

1. Quite often if no checks are required it's easy to undertest a PR
which leads to simple whitespace errors and such. Because of that I
created `pr_minimal_required_checks` which consists of PVP checks and
standard check which I will require on all PRs as minimum
2. Emma noticed that PRs are allowed to be merged when jobs are running
but none yet failed. This is an issue created by
#3580
and the fact that we want to run CI conditionally (for example we don't
want to run it when we only have DOCS changes) so we had to remove PR
trigger as required job in branch protection rules. This creates a
problem that without required check PR is allowed to be merged until a
test fails
3. Another problem that I spotted is that Documentation~ folder is
actually located inside package folder so this check implemented there
won't work correctly either way

The solution that this PR proposes is implementation of 2 independent
checks (triggered by expression trigger) where

1. `pr_minimal_required_checks` will trigger on all PRs targetting
develop or release branches. It consist of simplest PVP and standards
checks
2. `pr_code_changes_checks` will trigger on PRs that are modifying files
under relevant package or project related paths (see expression trigger)

Those jobs WON'T be required by the branch protection rules but I
introduced new GitHub action of Yamato PR Supervisor which WILL be
required and how it works is that it monitors checks running on PRs and
will fail if any of them fails or pass if all checks are green. In this
way we can have a workflow that both runs only necessary tests but also
gates PRs from merging too early.

Note that you can still use `/ci ngo` comment trigger to run the check
on draft branch or branches not targeting develop nor release branches

After this is approved and merged I will add this check as required on
PRs in branch protection rules

N/A

Comments were added to the jobs (soon I also plan to create doc
describing our CI practices/learnings)

Playtest)
I tested scenarios with code changes and without code changes and the
workflow works as expected

Will do
michalChrobot added a commit that referenced this pull request Sep 23, 2025
…ing PRs from merging when tests are running (#3691)

## Purpose of this PR
This is a backport of
#3682.
Same description apply

### Jira ticket
N/A

## Documentation
Same as original PR

## Testing & QA (How your changes can be verified during release
Playtest)
Same as original PR

## Backports
This is a backport of
#3682
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants